Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
2d10275 to
47a5ca9
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval is waiting on an engineering-owner decision. No current spec governs List.onReorder, ListItem.isReorderDisabled or the UIKit reorder model (rows slide aside, springs kept under Reduce Motion, no insertion line), so an owner needs to settle that contract and which record governs it: an amended List contract, or spec:AST-094 with its draft requirements approved. Once the governing record states it, any engineering owner, the author included, can record the decision by commenting /approve-spec <full head SHA>. These still need changes whatever that decision is:
- The diff against
mainis about 31,000 authored lines from the predecessor stack. Land those pull requests or update the branch so this one carries only the reorder change. - Keyboard focus on a reorder handle draws two focus rings, the handle's and the whole row's, and the handle's ring is clipped at the list's edge.
- The stacked touch row minimum heights change default List row geometry, which the List contract's FR2 says
paddedpreserves.
[Automated review]
| onPointerDown={(event: PointerEvent<HTMLButtonElement>) => | ||
| event.stopPropagation() | ||
| } | ||
| {...focusOutlineProps.focusVisible(styles.handle)}> |
There was a problem hiding this comment.
Keyboard focus here also lights the row: Item paints focusOutlineProps.focusWithin for any focused descendant, so a focused handle shows two 2 px rings (measured in Chromium), against architecture:interaction-modality INV3. The handle's own ring also loses its inline end to the list's overflow-inline: clip because of marginInlineEnd: -8. Excluding the handle from the row's focus-within match and keeping the ring inside the clip would leave one indicator.
| */ | ||
| export const listRowStyles = stylex.create({ | ||
| touchOneLine: { | ||
| minHeight: {default: null, [TOUCH]: listRowConsts.touchRowHeight}, |
There was a problem hiding this comment.
Every ListItem gets this minimum on a coarse pointer (applied in ListItem.tsx lines 456-460), so default lists grow from 41 px to 52 px rows on phones. component:List FR2 says "padded MUST preserve current header, list, and row geometry", so this needs that record amended or should stay out of this diff.
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
47a5ca9 to
d67ffca
Compare
…al List reorder
There was a problem hiding this comment.
Approval is waiting on an engineering-owner decision: no current spec governs List.onReorder, ListItem.isReorderDisabled or the UIKit reorder model (rows slide aside, springs kept under Reduce Motion, no insertion line), so an owner needs to settle that contract and which record governs it, an amended List contract or spec:AST-094 with its draft requirements approved. Once the governing record states it, any engineering owner, the author included, can record the decision by commenting /approve-spec <full head SHA>. These still need changes whatever that decision is:
- The diff against
mainis about 31,000 authored lines from the predecessor stack; land those pull requests or update the branch so this one carries only the reorder change. - Keyboard focus on a reorder handle draws two focus rings, the handle's and the whole row's, and the handle's ring is clipped at the list's edge.
- The stacked touch row minimum heights change default List row geometry, which the List contract's FR2 says
paddedpreserves. spec:AST-094FR7 cites an AST-090 record that does not exist in the repository.
[Automated review]
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
No AST-090 record exists in the repository, at this head or on main, so an owner cannot check what FR7 departs from. Cite the record where it lives, or drop the departure until it lands.
| * nothing, and a disabled row's handle stays focusable but inert | ||
| * @position Tests for List/useListReorder.ts and List/ListReorderHandle.tsx; | ||
| * the touch and mouse drags are proven in a real browser by | ||
| * ListReorder.interaction.browser.spec.ts |
There was a problem hiding this comment.
ListReorder.interaction.browser.spec.ts is not in the tree at this head (the SYNC note in useListReorder.ts line 44 names it too). Drop the reference or land the browser proof with this change.
|
On the predecessor stack: this pull request is stacked, as its first line says, and only its last commit (d67ffca, 10 files, +1,703 −7) is its own. It rebases onto main as each predecessor lands, and the diff then shrinks to that commit. |
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
|
Agreed: one implementation.
The reorder also now keys its motion on the resolver's touch presentation ( |
There was a problem hiding this comment.
Changes are needed before this can land, although the reorder contract and the drag's look now follow the current specs everywhere except reorder mode's missing hint. Please address these:
- Update the branch once #8138 and #8336 land: 38,226 authored lines change against
main, and 32,809 of them are that open stack. - Ship reorder mode's hint (Move up, Move down and Done) here, or land #8337 with this pull request, since
spec:AST-103FR4 andspec:AST-094FR39 need a way to move a row with a mouse and no drag. scripts/touch-support-bundle.test.mjsfails “is absent from the root entry” becausecreateReorderablebrings the gesture modules into the package root, so #8671 (or another approved change to that check) has to land first.- Add mouse browser tests for the drag, drop and cancel (
spec:AST-103FR3), here or by landing #8140 with this pull request.
[Automated review]
| [controller, isTouch], | ||
| ); | ||
| return ( | ||
| <span id={controller.hintId} hidden> |
There was a problem hiding this comment.
Reorder mode renders only these hidden instructions: no hint with move buttons and Done. A mouse user who clicks a handle then has nothing on screen to move the row or end the mode (spec:AST-103 FR4, FR4a; spec:AST-094 FR39). Fold the hint in, or land #8337 with this pull request.
| @@ -172,6 +172,9 @@ export * from './Outline'; | |||
| // Overflow list | |||
| export * from './OverflowList'; | |||
|
|
|||
| // Reorder (a collection's `reorderable` prop) | |||
| export * from './reorderable'; | |||
There was a problem hiding this comment.
This export brings the reorder engine, and through ListReorderTouch.ts 14 gesture modules (61,003 minified bytes), into the root entry, so main's scripts/touch-support-bundle.test.mjs fails “is absent from the root entry”. #8671, or another approved change to that check, has to land first.
| * Enters reorder mode for the item with `key` from the app's own UI, such | ||
| * as a Move item in its context menu or a toolbar button, as activating | ||
| * its handle does: the focus moves to the handle, which scrolls into view, | ||
| * and the mode's keys, hint, announcements and exits follow. A key that |
There was a problem hiding this comment.
This promises a hint that this revision does not render; keep “hint” only if #8337 lands together with this pull request.
There was a problem hiding this comment.
Changes are needed before this can land, although the reorder contract and the drag's look follow the current specs everywhere except reorder mode's missing hint. Please address these:
- Update the branch once #8138 and #8336 land: 38,474 authored lines change against
main, and 33,057 of them are that open stack. - Ship reorder mode's hint (Move up, Move down and Done) here, or land #8337 together with this pull request, since
spec:AST-103FR4 andspec:AST-094FR39 need a way to move a row with a mouse and no drag. scripts/touch-support-bundle.test.mjsfails "is absent from the root entry" becausecreateReorderablebrings the gesture modules into the package root, so #8671 (or another approved change to that check) has to land first.- Add mouse browser tests for the drag, drop and cancel (
spec:AST-103FR3), here or by landing #8140 with this pull request.
[Automated review]
| [controller, isTouch], | ||
| ); | ||
| return ( | ||
| <span id={controller.hintId} hidden> |
There was a problem hiding this comment.
Reorder mode renders only these hidden instructions: no hint with move buttons and Done. A mouse user who clicks a handle then has nothing on screen to move the row or end the mode (spec:AST-103 FR4, FR4a; spec:AST-094 FR39). Fold the hint in, or land #8337 with this pull request.
| @@ -172,6 +172,9 @@ export * from './Outline'; | |||
| // Overflow list | |||
| export * from './OverflowList'; | |||
|
|
|||
| // Reorder (a collection's `reorderable` prop) | |||
| export * from './reorderable'; | |||
There was a problem hiding this comment.
This export brings the reorder engine, and through ListReorderTouch.ts 14 gesture modules (61,052 minified bytes), into the root entry, so main's scripts/touch-support-bundle.test.mjs fails "is absent from the root entry". #8671, or another approved change to that check, has to land first.
| * Enters reorder mode for the item with `key` from the app's own UI, such | ||
| * as a Move item in its context menu or a toolbar button, as activating | ||
| * its handle does: the focus moves to the handle, which scrolls into view, | ||
| * and the mode's keys, hint, announcements and exits follow. A key that |
There was a problem hiding this comment.
This promises a hint that this revision does not render; keep "hint" only if #8337 lands together with this pull request.
|
Posted by Cindy's agent on her behalf: closed and reopened only to restart this PR's CI on the current workflow after tonight's queue-wide cancel; nothing in the PR changed. |
There was a problem hiding this comment.
Changes are needed before this can land: the List reorder follows current spec:AST-103 in most places, but the branch carries the open predecessor stack, conflicts with main, and misses four current AST-103 requirements. Please address these:
- Update the branch once #8138, #8336, #8106 and #8076 land: 36,294 authored lines change against
main, 30,875 of them that stack, and it now conflicts inTreeList.tsx,TreeListItem.tsxandroutes.lock.json. - Ship reorder mode's hint (Move up, Move down and Done) here or land #8337 with this, since AST-103 FR4 and
spec:AST-094FR39 need a way to move a row with a mouse and no drag. - Let a pen, and a finger where the primary pointer is fine, drag a row (AST-103 FR3, AST-094 FR43), and add
attachandpositions(AST-103 FR1, FR4b), here or by landing #8683 and #8719 with this. scripts/touch-support-bundle.test.mjsfails “is absent from the root entry” becausecreateReorderablebrings the gesture modules into the package root, so #8671 (or another approved change to that check) has to land first.- Add mouse and pen browser tests for the drag, drop and cancel (AST-103 FR3), here or by landing #8140 with this.
[Automated review]
| return () => {}; | ||
| } | ||
| const onPointerDown = (event: PointerEvent) => { | ||
| if (event.pointerType !== 'mouse' || event.button !== 0) { |
There was a problem hiding this comment.
This path takes only a mouse, and the hold attaches only under a coarse primary pointer (line 1866), so on a touchscreen laptop or with a desktop pen tablet a pen or a finger cannot drag a row. Current spec:AST-103 FR3 has a pen on a fine primary pointer drag without a hold and a finger keep the hold, and spec:AST-094 FR43 attaches gestures for every pointer; #8683 carries that change, so fold it in or land it with this.
| export function createReorderable< | ||
| TKey extends string | number = string | number, | ||
| TTo = number, | ||
| >(options: ReorderableOptions<TKey, TTo>): Reorderable<TKey, TTo> { |
There was a problem hiding this comment.
Current spec:AST-103 FR4b says the object exposes attach(container, {items}), and FR1 gives the options an optional positions(); this object has only start, and the options only onReorder and isReorderable, so the spec's own attach example does not type-check. Fold in #8719, land it with this, or have an engineering owner record on main that they ship later.
| * Off by default; with a mouse nothing moves. | ||
| * @default false | ||
| */ | ||
| hasAnimatedChanges?: boolean; |
There was a problem hiding this comment.
This boolean comes from the predecessor stack (#8106). Current component:List FR12 gives this job to an animatedLayout handle from useAnimatedLayout (spec:AST-105 FR1), and AST-105 FR6 says “no other API does this job; and it is not a visual boolean”, so shipping hasAnimatedChanges adds a second public shape that a later release would have to remove.
|
An offer that brings #8106's new head into this PR: offer/list-reorder-animated-layout fast-forwards this branch with four commits.
The List, reorder and motion unit tests, typecheck, lint and If you want it here, merge the branch. |
There was a problem hiding this comment.
Changes are needed before this can land: the merge with main resolves the earlier conflict, but the branch still carries the open predecessor stack, fails main's root-entry bundle test in CI, and misses parts of current spec:AST-103. Please address these:
- Update the branch once #8138, #8336, #8106 and #8076 land: 36,308 authored lines change against
main, about 30,889 of them that stack. - Land #8671, or another approved change to
scripts/touch-support-bundle.test.mjs, first:createReorderable's root export brings 14 gesture modules into the root entry, andtest-nodefails on it. - Ship reorder mode's hint (Move up, Move down and Done) here, or land #8337 with this, since AST-103 FR4 and
spec:AST-094FR39 need a way to move a row with a mouse and no drag. - Let a pen, and a finger where the primary pointer is fine, drag a row (AST-103 FR3, AST-094 FR43), and add
attachandpositions(FR1, FR4b), here or by landing #8683 and #8719 with this. - Show the insertion line as soon as reorder mode starts (FR4), as your offered branch does, and add mouse and pen browser tests for the drag (FR3), here or with #8140.
[Automated review]
| [controller, isTouch], | ||
| ); | ||
| return ( | ||
| <span id={controller.hintId} hidden> |
There was a problem hiding this comment.
Reorder mode renders only this hidden description: no hint with move buttons and Done, so a mouse user who clicks a handle has nothing on screen to move the row or end the mode (spec:AST-103 FR4, FR4a; spec:AST-094 FR39). Fold the hint in, or land #8337 with this pull request.
| return () => {}; | ||
| } | ||
| const onPointerDown = (event: PointerEvent) => { | ||
| if (event.pointerType !== 'mouse' || event.button !== 0) { |
There was a problem hiding this comment.
This path takes only a mouse, and the hold attaches only under a coarse primary pointer (line 1866), so a pen, or a finger on a touchscreen laptop, cannot drag a row. Current spec:AST-103 FR3 has a pen on a fine primary pointer drag without a hold and a finger keep the hold, and spec:AST-094 FR43 attaches gestures for every pointer; #8683 carries the change.
| */ | ||
| private placeModeLine(list: HTMLElement): void { | ||
| const row = this.rowOf(this.modeHandle); | ||
| if (row == null || this.lastStep == null) { |
There was a problem hiding this comment.
With lastStep null the line is cleared, so entering reorder mode shows no insertion line until the first arrow press. Current spec:AST-103 FR4 has the line mark the item's place for as long as it is in reorder mode; the fix described in comment 6467628718 would close this.
| export function createReorderable< | ||
| TKey extends string | number = string | number, | ||
| TTo = number, | ||
| >(options: ReorderableOptions<TKey, TTo>): Reorderable<TKey, TTo> { |
There was a problem hiding this comment.
Current spec:AST-103 FR4b says the object exposes attach(container, {items}), and FR1 gives the options an optional positions(); this object has only start, and the options only onReorder and isReorderable, so the spec's own attach example does not type-check. Fold in #8719, land it with this, or have an owner record on main that these ship later.
| @@ -172,6 +172,9 @@ export * from './Outline'; | |||
| // Overflow list | |||
| export * from './OverflowList'; | |||
|
|
|||
| // Reorder (a collection's `reorderable` prop) | |||
| export * from './reorderable'; | |||
There was a problem hiding this comment.
This export brings the reorder engine, and through ListReorderTouch.ts 14 gesture modules (61,052 bytes), into the root entry, and CI's test-node (2/4) fails scripts/touch-support-bundle.test.mjs “is absent from the root entry” on it. #8671, or another approved change to that check, has to land first.
| * Off by default; with a mouse nothing moves. | ||
| * @default false | ||
| */ | ||
| hasAnimatedChanges?: boolean; |
There was a problem hiding this comment.
This boolean comes from the predecessor stack (#8106). Current component:List FR12 gives this job to an animatedLayout handle from useAnimatedLayout (spec:AST-105 FR1), and AST-105 FR6 says “no other API does this job; and it is not a visual boolean”, so shipping hasAnimatedChanges adds a public shape that would need a breaking removal.
There was a problem hiding this comment.
Changes are needed before this can land. The List reorder follows current spec:AST-103 in most places, but the branch conflicts with main, still carries its predecessor stack, and misses parts of AST-103. Please address these:
- Rebase onto
main: the branch conflicts in 82 files, including add/add conflicts with the gesture host and touch foundation thatmainhas since landed in another form, so no build or test workflow ran on this revision. - Land or re-cut #8138, #8336 and #8106 first, so this branch holds only the reorder: about 30,800 of its 36,313 changed lines belong to that stack.
- Render reorder mode's hint, with move buttons and Done, here or by landing #8337 with this; AST-103 FR4 and FR4a and
spec:AST-094FR39 require a way to move a row without dragging (listReorderEngine.tsx:1869-1873). - Show the insertion line at the row as soon as reorder mode starts, not only after the first arrow press (AST-103 FR4;
listReorderEngine.tsx:1552-1557). - Let a pen on a fine primary pointer drag without a hold, and a finger drag after the hold on any device (AST-103 FR3,
spec:AST-094FR43;listReorderEngine.tsx:845,:1865-1868). - Add
positions()to the options andattach(container, {items})to the object, as AST-103 FR1 and FR4b require (createReorderable.ts:47-89). - Make the bundle tests pass: the reorderable entry brings 14 gesture modules into the root, so after the rebase list it among
main's gesture-using public imports inscripts/touch-support-bundle.test.mjs. - Replace
List.hasAnimatedChangeswith theanimatedLayouthandle thatcomponent:ListFR12 requires (List.tsx:88). - A mouse click in a link row's padding now follows the link for every
ItemandListItemwithhref(Item.tsx:749-768): this desktop behavior change needs an owner decision, or a gate to touch input. - Call the shared
isScrollCapableOverflowinstead of the copied scroll-container check, which dropsoverlay(listReorderEngine.tsx:307-313). - Add browser tests for a mouse drag and for a pen drag on fine and coarse primary pointers (AST-103 FR3 verification).
[Automated review]
| return ( | ||
| <span id={controller.hintId} hidden> | ||
| {t('@astryx.list.reorderHint')} | ||
| </span> | ||
| ); |
There was a problem hiding this comment.
Reorder mode renders only this hidden description, so after a mouse click on a handle there is nothing to click next. Current spec:AST-103 FR4 and FR4a require a hint anchored to the row, with the axis's move buttons and Done, and spec:AST-094 FR39 requires a way to move without dragging. Render it here, or land #8337 together with this.
| if (row == null || this.lastStep == null) { | ||
| this.clearModeLine(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
With lastStep null this clears the line, so reorder mode shows no insertion line until the first arrow press. AST-103 FR4 has the line mark the item's place for as long as the mode lasts. Place it at the row's own gap when the mode starts.
| if (event.pointerType !== 'mouse' || event.button !== 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Only a mouse takes this immediate path, and the hold is attached only under a touch presentation (lines 1865-1868). A pen on a desktop tablet, or a finger on a touchscreen laptop, therefore cannot drag at all. AST-103 FR3 gives a pen on a fine primary pointer a drag without a hold, and a finger the hold; spec:AST-094 FR43 says gestures attach for every pointer, "not only a coarse primary pointer".
| export interface ReorderableOptions< | ||
| TKey extends string | number = string | number, | ||
| TTo = number, | ||
| > { | ||
| /** | ||
| * Called once per committed move: a drop, or each move in reorder mode. | ||
| * Apply it by rendering the items in their new order. A cancelled move calls nothing. The latest function given is the one | ||
| * called, so the object may be made on every render. | ||
| */ | ||
| onReorder: (move: ReorderMove<TKey, TTo>) => void; | ||
| /** | ||
| * Whether the item with `key` can move. An item it rejects shows no handle | ||
| * and cannot be picked up; one at either end of the collection also bounds | ||
| * the others, so nothing drops beyond it and a keyboard move into it is | ||
| * refused. Every item moves without it. | ||
| */ | ||
| isReorderable?: (key: TKey) => boolean; | ||
| } |
There was a problem hiding this comment.
Current AST-103 FR1 adds an optional positions() here, and FR4b gives the object attach(container, {items}) beside start. Neither carries an implementation-gap note in the record, so the public shape is incomplete.
| @@ -172,6 +172,9 @@ export * from './Outline'; | |||
| // Overflow list | |||
| export * from './OverflowList'; | |||
|
|
|||
| // Reorder (a collection's `reorderable` prop) | |||
| export * from './reorderable'; | |||
There was a problem hiding this comment.
This export brings the reorder engine, and through ListReorderTouch.ts 14 gesture modules, into the root entry. The head's scripts/touch-support-bundle.test.mjs fails "is absent from the root entry", and main's version fails "is in no public import that uses no gesture". architecture:react-component-runtime INV13 allows an opt-in constructor to carry its touch modules, so after the rebase, list the reorderable entry among the gesture-using public imports in main's test.
| * Off by default; with a mouse nothing moves. | ||
| * @default false | ||
| */ | ||
| hasAnimatedChanges?: boolean; |
There was a problem hiding this comment.
Current component:List FR12 says "List MUST take an animatedLayout handle", and spec:AST-105 FR6 says no other API does this job. This boolean would be a second API for it. Drop it and adopt the handle, as #8106's newer head does.
| if (onClick == null && href != null && !isLinkRoot) { | ||
| // A link row's own box follows its link: a click in its padding, or a | ||
| // tap in its touch hit area under a finger, goes to the row's anchor | ||
| // with its modifier keys, so the link component and the browser decide | ||
| // the destination and the tab as for a click on the label. | ||
| (e.currentTarget as HTMLElement) | ||
| .querySelector<HTMLElement>('[data-astryx-row-link]') | ||
| ?.dispatchEvent( | ||
| new MouseEvent('click', { | ||
| bubbles: true, | ||
| cancelable: true, | ||
| ctrlKey: e.ctrlKey, | ||
| metaKey: e.metaKey, | ||
| shiftKey: e.shiftKey, | ||
| altKey: e.altKey, | ||
| button: e.button, | ||
| }), | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This runs for every pointer, so a mouse click in a link row's padding now follows the link for every Item and ListItem with href. At the base it does nothing. No record covers this desktop change. Gate it to touch input (the coarse hit area it was added for), or cite the owner decision.
| function isScrollContainer(element: HTMLElement): boolean { | ||
| const {overflowY} = getComputedStyle(element); | ||
| return ( | ||
| (overflowY === 'auto' || overflowY === 'scroll') && | ||
| element.scrollHeight > element.clientHeight | ||
| ); | ||
| } |
There was a problem hiding this comment.
This copies isScrollCapableOverflow from hooks/scrollGeometry.ts but leaves out overlay, so autoscroll can skip a scroller the rest of Core treats as one. Call the shared predicate instead.
| * not raised. | ||
| */ | ||
| const MOVING_OPACITY = '0.6'; | ||
| /** The insertion line's thickness, px. */ |
There was a problem hiding this comment.
Non-blocking: no declaration follows this comment any more (the constant moved to ListReorderHandle.tsx). Delete it.
|
/approve-spec 2cb3df9 |
There was a problem hiding this comment.
Changes requested. The reorder follows spec:AST-103 closely and its record edits are owner-approved for this head, but a few required behaviors are missing and two checks need fixing.
- Reorder mode shows no hint with move buttons and Done (
listReorderEngine.tsx:1880-1884), so a mouse user cannot move a row without dragging, whichspec:AST-103FR4 and FR4a andspec:AST-094FR39 require; ship the hint here or merge #8337 together with this pull request. - Reorder mode draws no insertion line until the first arrow press (
listReorderEngine.tsx:1559-1564, pinned byListReorder.test.tsx:562-563), while FR4 has the line mark the item's place for the whole mode. - A pen on a fine primary pointer and a finger on a fine-pointer device cannot drag (
listReorderEngine.tsx:853accepts only a mouse and:1876-1879attaches the hold only under a touch presentation), which FR3 andspec:AST-094FR43 require. isScrollContainer(listReorderEngine.tsx:317-323) copiesisScrollCapableOverflowfromhooks/scrollGeometry.tswithoutoverlay; call the shared predicate instead.- Miss: the layout Effect at
ListItem.tsx:436-443does only the row's detach work, which belongs in a callback ref underarchitecture:react-component-runtimeINV6. - Miss: the title says rows reorder "the way an iOS table does", but the drag follows
design:ordered-collection-reorderingand departs from UIKit (motion/uikit/reorder.ts:23-29); please retitle it to what the change does. - FR3's verification needs browser tests with a mouse and with a pen on a fine and a coarse primary pointer; the mouse drag is covered only in jsdom.
- FR2's verification needs the measured bundle cost of
createReorderablerecorded in the description.
The size is fine as a behavior module. CI's build and test workflows have not run on this head yet.
[Automated review]
| () => (isTouch ? controller.attachTouch() : undefined), | ||
| [controller, isTouch], | ||
| ); | ||
| return ( |
There was a problem hiding this comment.
Reorder mode renders no hint here, only the hidden instructions. spec:AST-103 FR4 and FR4a need a hint anchored to the row with move buttons and Done for as long as the mode lasts, and spec:AST-094 FR39 needs a mouse path that does not drag. Without it, a mouse user who clicks the handle has nothing to click next. Ship the hint with this pull request (it is built in #8337) or merge the two together.
There was a problem hiding this comment.
The hint ships in #8337, which merges together with this one. The description's stack lines now say so.
| * gap above it after a move up, and below it after a move down. None before | ||
| * a move. | ||
| */ | ||
| private placeModeLine(list: HTMLElement): void { |
There was a problem hiding this comment.
This clears the line while lastStep is null, so the mode shows no insertion line until the first arrow press. spec:AST-103 FR4: "while the item is in reorder mode, the same accent insertion line a drag shows marks the item's place". Place the line at the row when the mode starts.
There was a problem hiding this comment.
Fixed in 9a1afa9: the line marks the row's place from the start of the mode, in the gap above it until a move.
| const handle = handleOf('Inbox'); | ||
| handle.focus(); | ||
| await user.keyboard(' '); | ||
| // No line before a move. |
There was a problem hiding this comment.
This asserts the opposite of spec:AST-103 FR4 (the line marks the item's place for the whole mode). Flip it once the line shows at the start of the mode.
There was a problem hiding this comment.
Flipped in 9a1afa9: it now asserts the line from the start of the mode, and it fails on the previous head.
| return () => {}; | ||
| } | ||
| const onPointerDown = (event: PointerEvent) => { | ||
| if (event.pointerType !== 'mouse' || event.button !== 0) { |
There was a problem hiding this comment.
Only a mouse drags without a hold, and the hold attaches only under a touch presentation (:1876-1879). spec:AST-103 FR3 gives a pen on a fine primary pointer a drag with no hold and a finger the hold on every device, and spec:AST-094 FR43 attaches gestures for every pointer. Accept a pen here when the primary pointer is fine, and attach the hold for touch input everywhere.
There was a problem hiding this comment.
Fixed in 9a1afa9: a pen drags at once where the primary pointer is fine, and the hold attaches under every primary pointer (a finger always, a pen where it's coarse). New browser proofs cover a mouse, and a pen on a fine and a coarse primary pointer.
| return row.offsetTop - base; | ||
| } | ||
|
|
||
| function isScrollContainer(element: HTMLElement): boolean { |
There was a problem hiding this comment.
This copies isScrollCapableOverflow (hooks/scrollGeometry.ts:64) but leaves out overlay, so the two checks already disagree and a fix to the shared one would miss the reorder. Call the shared predicate.
There was a problem hiding this comment.
Fixed in 9a1afa9: it calls isScrollCapableOverflow.
| // shows the row as it was on screen. Only a row its list reported for the | ||
| // commit can exit. So when StrictMode replays this cleanup on a new row, | ||
| // during its entrance, it writes nothing. | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
Miss: this layout Effect only does the row's detach work (writing its running animations inline for the exit). architecture:react-component-runtime INV6 gives node detachment to a callback ref, and a ref cleanup runs at the same point of the commit, while the row is still in the document. Merge a callback ref into mergedRef that calls keepPresentation in its cleanup, and drop the Effect.
|
/approve-spec 9a1afa9 |
There was a problem hiding this comment.
Changes requested. The reorder follows spec:AST-103 closely, the pen and finger routing, the opening insertion line and the earlier shared-path and Effect findings are fixed, and the record edits are owner-approved for this head, but three things still need fixing:
- Reorder mode shows no hint with move buttons and Done (
listReorderEngine.tsx:1896-1900), which currentspec:AST-103FR4 and FR4a andspec:AST-094FR39 require, so a mouse or touch user who activates a handle cannot move the row without a keyboard; ship the hint here or land #8337 as one change with this one. keepPresentationOnDetach(ListItem.tsx:400-409) runs for every row the list reported for a commit, so when a staying row's ref changes identity during ananimatedLayoutchange (an inline callbackref, for example) its running animations are written as inline styles that nothing clears; run it only for rows that are leaving and add that case to the tests.- Miss:
utils/rowMotion.ts:138-151copiesmotion/animator.ts's privateisIdentityTextanddefaultNow, already with different tolerances; export the animator's helpers and call them instead.
Smaller notes, such as comments that still say the touch hold is coarse-pointer only and a test whose title contradicts the code, are in the review.
[Automated review]
| [controller, isTouch], | ||
| ); | ||
| return ( | ||
| <span id={controller.hintId} hidden> |
There was a problem hiding this comment.
Reorder mode still renders only these hidden instructions: nothing anchored to the row offers move buttons or Done. Current spec:AST-103 FR4 and FR4a require that hint for as long as the mode lasts, and spec:AST-094 FR39 requires a mouse path that does not drag. Ship the hint here, or land #8337 as one change with this one.
| * commit can exit. So when StrictMode replays the detach of a new row, during | ||
| * its entrance, it writes nothing. | ||
| */ | ||
| function keepPresentationOnDetach(row: HTMLLIElement | null) { |
There was a problem hiding this comment.
isReportedRow is true for every row the list reported for the commit, not only for rows that leave. When a staying row's merged ref changes identity in that commit (an inline callback ref, or useTouchHitArea returning a new ref), React runs this cleanup and keepPresentation writes the row's running animations inline, where nothing clears them. Run it only for a row the list reports as leaving, and add an inline-ref case to listLayoutUpdates.test.tsx.
| * hold the same channel off rest, such as a swipe's x on `translate`. Only | ||
| * identity values are ours to clear. | ||
| */ | ||
| function isIdentity(property: string, value: string): boolean { |
There was a problem hiding this comment.
Miss: this repeats motion/animator.ts's private isIdentityText (and motionNow repeats its defaultNow), already with a different tolerance, so a fix to one misses the other. Export the animator's helpers and call them here.
There was a problem hiding this comment.
Fixed in 5e893e4: the animator exports isIdentityText and motionNow, and rowMotion calls them.
A List that takes the handle useAnimatedLayout returns animates its row changes on one shared motion, for every pointer. An inserted row enters, a removed row collapses in place, and a row that crosses rows moving the other way rides above them. The rows after a change, and the content after the list, glide to their new places. Rows match across renders by their itemKey; a row without one changes at once. The row marks the update module writes (item key, leaving, travelling) live in their own private-attribute owner modules. A list under the handle isolates for every pointer, so a sliding row stays below a page-level sticky header. The List and ListItem records now say the row updates and itemKey are implemented, and their verified_by lists the files that verify them.
There was a problem hiding this comment.
Changes requested. The List reorder follows current spec:AST-103 closely, and the staying-row style fix and the shared animator helpers from the last round are in, but one thing still needs fixing:
- Reorder mode shows no hint with move buttons and Done (
listReorderEngine.tsx:1897-1901), which currentspec:AST-103FR4 and FR4a andspec:AST-094FR39 require, so a mouse or touch user who activates a handle cannot move the row without a keyboard; ship the hint in this pull request, since #8337 is built on an older head of this branch and nothing makes the two land together.
Smaller notes, such as two stale test comments and a comment that restates isPressed, are in the review.
[Automated review]
| [controller, isTouch], | ||
| ); | ||
| return ( | ||
| <span id={controller.hintId} hidden> |
There was a problem hiding this comment.
Reorder mode still renders only these hidden instructions: nothing anchored to the row offers move buttons or Done. Current spec:AST-103 FR4 and FR4a require that hint for as long as the mode lasts, and spec:AST-094 FR39 requires a mouse path that does not drag. #8337 builds the hint on an older head of this branch; ship it here.
| * Enters reorder mode for the item with `key` from the app's own UI, as | ||
| * activating its handle does. A Move item in a context menu or a toolbar | ||
| * button can call it. The focus moves to the handle, which scrolls into | ||
| * view, and the mode's keys, hint, announcements and exits follow. A key |
There was a problem hiding this comment.
This promises a hint that this revision does not render; keep "hint" here once the hint ships in this pull request.
There was a problem hiding this comment.
The hint ships here now, so the word stays.
| (text !== '' ? text : labelTextOf(ref.current, labelID ?? null)); | ||
| return controller?.registerHandle(ref.current, itemKey, spoken); | ||
| }, [controller, itemKey, callerLabel, text, labelID]); | ||
| // Pressed while its row is in reorder mode. |
There was a problem hiding this comment.
Non-blocking: this restates isPressed (architecture:knowledge-contracts INV29). Delete it.
|
/approve-spec 5e893e4 |
createReorderable({onReorder, isReorderable}) makes a collection's
reorder behavior, and List takes it as `reorderable`. Each ListItem
with an itemKey gets a reorder handle at its end, and each committed
move calls onReorder({key, to}) once.
Under a finger, a 150 ms hold on a handle picks its row up, and a
flick on it still scrolls. The rows stay still while a dimmed source,
a preview and an insertion line show the move. On release they glide
on the shared settle role. A pen holds the same way where the primary
pointer is coarse. A mouse, or a pen where the primary pointer is
fine, drags a handle at once. A click, Space or Enter turns on reorder
mode: the insertion line marks the row's place, the arrow keys move
the row and announce its place, and Escape ends the mode.
The reorder's machinery ships with the object, so a List without
`reorderable` loads none of it. The UIKit table's reorder values and
the drop model come in here with their first user. With an
animatedLayout handle, a drag's commits stay the reorder's own. The
CLI lists createReorderable with the hooks.
While a reorderable List row is in reorder mode, a hint anchored below its handle shows Move up, Move down and Done until the mode ends. Each move does what its key does, with the same announcement, and leaves the focus on the handle. Done ends the mode as Escape does, keeping the moves. So a pointer or a finger can reorder by taps alone, as WCAG 2.5.7 asks. A screen reader that moves its focus into the hint keeps the mode, and the focus goes back to the handle. The hint shares the keyboard hint's layer: hooks/anchoredHint.tsx holds the anchored hint both render, and useKeyboardHint renders its hint through it unchanged. The hint joins the shared dismissal stack, so an Escape in it ends the mode and leaves a Dialog below it open.
A row named by its place, as ListInput names its records, gets a new name after each move. The handle now records its name apart from its registration, so a new name leaves the row in reorder mode. A move is announced by the names the rows have when it lands. Comments now say that a drag ends reorder mode with its announcement, and test comments match the hold and blur behavior.
There was a problem hiding this comment.
Approved pending CI. CI has not run on this head yet (the pull request is a draft), so it is not ready to merge until its required checks pass.
List's reorder now follows current spec:AST-103 for everything it builds: reorder mode's hint with Move up, Move down and Done ships here, so taps or clicks alone can reorder a row, and a move that renames its row keeps the mode.
Next step:
- This branch carries #8106's commit unchanged, so merge it after #8106, as the description says. The stack lines in #8143 and #8163 still list #8337, which is now folded in here.
One non-blocking comment suggestion is inline.
[Automated review]
| * @output renderAnchoredHint and anchoredHintStyles: the anchored hint layer | ||
| * Core's keyboard hints share. It is a small surface in the top layer, | ||
| * below the start of the element it is anchored to. It has the popover | ||
| * background, the element radius, the low shadow and supporting text. It | ||
| * holds a row of key glyphs and a label |
There was a problem hiding this comment.
Non-blocking: the last three sentences list the surface's style values, which the style object below already states (architecture:knowledge-contracts INV29). Suggest ending @output at "the anchored hint layer Core's keyboard hints share".
|
/approve-spec ee13b42 |
Stack: #8106 → #8139 → #8143 → #8163
#8106 is held for the timing batch, so this merges after it. It holds three commits. List's reorder comes first (2931552). Reorder mode's hint, with its move buttons and Done, comes second (63d7d51): it was #8337, folded in as the stack review asked, its patch unchanged (patch-id d56e6e69d1). The fixes for the last review come third (ee13b42). Review only this PR's change: vjeux/astryx@08652ac...ee13b42
Why
People reorder playlists, to-do lists and settings on their phones.
Listcan't do it, so apps build their own, often without a keyboard or screen-reader path.What led to it
This is part of the UIKit-parity program, which makes Astryx's components behave like their iOS counterparts on touch screens.
spec:AST-103defines a reorderable List:createReorderable({onReorder})makes the behavior,Listtakes it asreorderable, and eachListItemwith anitemKeygets a handle.onReorder({key, to})once.This builds
spec:AST-103FR1 (butpositions()andlabels) and FR2 to FR8. AST-103 gains "Implementation gap: not built" notes for FR1'spositions()and FR4b'sattach, beside itslabelsnote.component:List's readiness line saysreorderableis stable and built, but for itslabelsandpositions.family:overlay-dismissallists the hint among its members. The UIKit table's measured reorder values and the drop model come in here with their first user. With ananimatedLayouthandle, a drag's commits stay the reorder's own. The CLI listscreateReorderablewith the hooks.Options and tradeoffs
Why this choice
design:ordered-collection-reorderingchose still rows and a line, because they need no layout work during the drag and look the same everywhere. The hold is UIKit's measured 150 ms. Only the handle is added (spec:AST-103FR5), so rows keep their layout. DEC-5 chose the hint, because it shows the moves where the row is, without opening a menu. It is the keyboard hint's own layer, and each button's tap area grows toward 44 px on touch through the shared touch hit area. The API isspec:AST-103FR1's: one behavior object, with no hook twin.How to check it
In Storybook
Core/List, on a phone or with touch emulation:Gates at ee13b42, and at the stack's top (#8163), which carries it:
experimental-isolation, 44 of 44).check:repo,check:knowledge,check:changesets,check:use-client,check:i18n-catalog,check:i18n-runtimeandcheck:package-boundaries: 0.createReorderable, 303 of 303 here. At the top, the full UI project, 21,206 tests, passes.Bundle cost
Measured with the PR size report's build options on the built package (the eager chunks, minified, gzip): List and ListItem import 70.0 KB. With
createReorderablethey import 114.8 KB, so the reorder adds 44.8 KB where it is used. A List withoutreorderableimports no reorder, gesture or animator module, only the small module that names the object's key.Writing rubric results
Over
component:List(the readiness line andverified_by),spec:AST-103(two gap notes) andfamily:overlay-dismissal(the hint joins the members):Pull request size
This is a behavior-module pull request: 7,749 authored lines in 50 files: 4,041 of source, 3,292 of tests, and the rest stories, strings, docs and the API report. The reorder engine, its touch layer, its model, its handle and its hint work only together, so they land as one module.
Evidence
On iOS 26.2 Safari: a held drag, the drop with the main thread blocked, and tap, Move down, Done. The last was captured before Done took the hint's text role, so Done there is darker and bolder than it is now.